-
Notifications
You must be signed in to change notification settings - Fork 263
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix XMLHttpRequest issues #3237
Conversation
@@ -56,7 +56,9 @@ public void _getResponseHeaders(JSObject jheaders) { | |||
|
|||
responseHeaders.forEach( | |||
(key, value) -> { | |||
jheaders.setMember(key, value); | |||
if (!key.equals(":Status")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this isn't mentioned explicitly in the linked bug reports, I have a couple questions:
- What problems does it cause for this pseudo-header to be present?
- Are there other pseudo-headers that should also be filtered out to avoid similar problems?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops! That should have been included in the fetch
bug report.
:Status
is an intentionally invalid HTTP header which lets the macro writer explicitly set the response code. It is also used internally to get the status code from the request processing out to the wrapper layer. It could use an optional, or similar data type, but exposing those to the macro writers would be a bit of a trick. By specification, invalid HTTP headers must not be returned to the client, so it must be stripped out before the headers are returned. If the response is wrapped in a javascript Response
object (as the fetch
API does), Response
's constructor will throw an exception on invalid headers. Note this is different from undefined / extension headers (valid headers must not start with :
).
The problem is the header filtering happens when the response body is received. If there is an exception thrown between when the header default values are set and the response body is received, the body-received function is not called, so the headers do not get filtered. When fetch
then tries to handle the failed request, it chokes on the invalid header.
So to sum up:
- It breaks the
fetch
API - No.
That said, I think it might still have a problem outside the fetch
API on internal server errors. Probably need to refactor it slightly to always explicitly filter the headers without calling the body-received function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, nope, I can't see why the MTXMLHttpRequest::recv
doesn't get called before the headers get retrieved. Especially given that the critical onreadystatechange
call which triggers the javascript-side header processing happens after the headers should be handled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @perkinslr)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @perkinslr)
Identify the Bug or Feature request
#3236
#3235
Description of the Change
Changes the decoding for lib:// targets to UTF-8
Changes
fetch
to anasync
function, and internallyawait
s the body of the Request object.Filters out the pseudo-header
:Status
from the responseHeaders for thefetch
API if the underlying response fails.Possible Drawbacks
Binary data returned from lib:// targets will be improperly decoded. Binary data should be stored in asset:// instead.
Documentation Notes
Sending
body:
argument viafetch
API now works.lib://
URIs now return sensible output.Release Notes
Examples:
body:
argument viafetch
API now works.lib://
URIs now return sensible output.This change is